Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul Custom Fields for Devices to support any underlying type #443

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tagur87
Copy link
Contributor

@tagur87 tagur87 commented Aug 1, 2023

feat: allow any custom field type in netbox_device

Updates the netbox_device resource to allow for any custom field
type to be set. This is done by using a string field with the
jsonencode() and jsondecode() terraform functions.

This allows for any complex custom fields or data types that are needed,
since the underlying types don't matter and are unmarshalled using
map[string]interface{}.

To set custom fields using this new method, use the following syntax.

resource "netbox_device" "test" {
  name = "device1"
  tenant_id = netbox_tenant.test.id
  role_id = netbox_device_role.test.id
  device_type_id = netbox_device_type.test.id
  site_id = netbox_site.test.id
	custom_fields = jsonencode({
		"text_field" = "81"
		"bool_field" = true
	})
}

feat: allow any custom field type in netbox_devices data source

Update the data source netbox_devices to use the string based
schema for the custom_fields attribute.

This allows for any underlying type to be accessed by using the
jsondecode() terraform function.

For example, to access custom fields, the following code can be used.

data "netbox_devices" "test" {
	filter {
		name  = "name"
		value = "device1"
	}
}

output "device_field" {
	value = jsondecode(data.netbox_devices.test[0].custom_fields).field1
}

feat: Add funcs for custom fields of any type

Create functions for overahauling custom fields to support multiple
underlying types.

Create function for the schema with using TypeString instead of
TypeMap. This allows for complex json types using jsonencode/jsondecode.

Create handleCustomFieldUpdate function and tests for it. This function
takes in the results of a d.GetChange funtion old & new, which
allows us to compare old and new values, and update the custom_fields
field in the api. Add unit test to validate functionality.

handleCustomFieldRead and tests were added to handle the reading of
custom fields and setting the field to "" if all the custom fields
are nil. Adds tests to validate the functionality.

These changes may be breaking, however have not fully tested that part. Not sure how best to do so.

Fixes #416
Fixes #409

@fbreckle
Copy link
Collaborator

fbreckle commented Aug 2, 2023

Judging by the branch name, you have already noticed that this is a major overhaul of a part of this provider.

I will counter this PR with some thoughts about the current state of custom fields (CFs) in this provider. Note that since I do not use custom fields personally, my insights in this topic might be somewhat limited.

History

Custom fields quickly became a "I have to tackle this eventually" topic in this provider, as shown by the number of issues and PRs about this topic. Since it is also a somewhat complex topic, I historically prioritized "lower hanging fruit" like new resources/data-sources and small fixes and the occasional support for a new netbox version over "properly handle custom fields".
Now, obviously, changing the user-interface of CFs is going to be a major breaking change for many users, so changes should be well thought out and then applied to all resources.
A while ago, we had an implmentation PR where someone basically took the solution of another terraform provider (#235).

Netbox

Netbox supports custom fields of various types, including non-basic types like "multiple selection". I did not look at the API implementation of it yet, but I got the impression that it basically takes any object and checks at runtime whether the value of a given custom field matches the type defined in netbox.
Example: Let F1 be a custom field, then the API will accept {"F1": "foo" } as value of the CF if the type of F1 is "string", but it will respond with an error if F1 is of type "list".
So, API-wise, we do not have to (neither CAN we) explicitly provide the type of the CF when sending it to netbox. Let this be Assumption A1. Correct me if I'm wrong here.

Current state of the provider

Currently, we assume that the custom field is a string, which seems to work somewhat okay for string types (obviously) and seems to be wonky at best, broken at worst with other types of CFs.

Desired state of the provider

Well, that is a good question. Generally, all types of custom fields should be supported by the provider.

Over the years, I thought about this here and there and these were the questions I wanted to answer (given A1 above):

  1. Do we even need to validate the value of CFs in the provider before sending it to netbox? Why?

In that case, the type of of the CF must be known at plan-time.

  1. Can we somehow get the type from the upstream API at runtime?

  2. Do we have the user provide the type in the HCL code as in Add custom fields for prefix #235?

  3. Do we use a type field with jsonencode?

  4. Do we use a separate block per CF field type?
    E.g.
    custom_field_list { name = "foo" value = ["this", "is a terraform", "native list"] }
    This way, we could use native hcl data structures and do the actual JSON encoding in the provider code instead of HCL

  5. The version implemented here includes lots of raw json on the custom_field attribute level.
    Is this stable enough?
    Can we continue using custom_fields as a map fo strings to json-ified strings?

All this in addition to the usual considerations about best experience for the user etc.

Conclusion

Ideally, I want to handle the custom fields problem exactly once, in a global, sustainable way, not in small steps.

I will mark this as draft, similar to #428 while we discuss this.

@tagur87
Copy link
Contributor Author

tagur87 commented Aug 2, 2023

@fbreckle - thanks for taking the time to review this and provide such a clear response of the current state of CFs along with questions you have. I figured this was a major breaking change from the users perspective, since it's changing the behavior of the field. I thought doing it incrementally might be easier to handle, but i understand your perspective of doing a global change.

I will do my best to answer your questions to the best of my current understanding.

First of all, your assumption (A1) of how Netbox handles custom fields is correct. Netbox will check the types of the custom fields, and return an error from the API if the type for the specific custom field is incorrect.

1. Do we need to validate the CFs before sending?
I don't feel we do. Since cannot get the type of the custom field from the API without some major filtering (see point 2), I feel that the validation of the custom field should be on the user to do, based on their specific custom field implementation. The API errors should be able to lead them to format them correctly. But that's just my opinion.

2. Can we somehow get the type from the upstream API at runtime?
I'm sure it is possible, but this would mean that we have the query the custom field endpoint for each type that is used before we can even use them. For example, if we wanted to use 4 custom fields in a device resource, then we would have to make 4 queries to the custom field endpoint to get the type for each custom field. This is possible, but adds a lot of extra calls per resource that uses custom fields.

3. Do we have the user provide the type in the HCL code as in
Again, is possible, but back to point two, would have to have a way to look this up.

4. Do we use a type field with jsonencode?
I am not sure how this could work....the custom fields are just a json structure based on the name of the custom field, which goes back to the underlying type to know if it matches the type properly.

5. Do we use a separate block per CF field type?
This could be possible, but I think this could get very complicated. We would have to aggregate them all and then make the update call to the api.

6. Is the jsonencode() usage stable enough?
It seems to be. The errors returned by the API are pretty verbose and can help you understand where the errors lie. Also the code as currently implemented handles the unsetting of the fields if a field in the json is removed. Which doesn't happen by default.

If this provider was using the terraform-plugin-framework, there may be some other options in the schema to handle some of these complex types better. But with the SDKv2, I think our options are pretty limited.

@tagur87
Copy link
Contributor Author

tagur87 commented Aug 2, 2023

Also, for reference, here is the structure of every single custom field type in json form, for reference. All of their values when empty use null as well.

{
    "custom_fields": {
        "boolean": true,
        "date": "2023-08-02",
        "decimal": 1.2,
        "integer": 1,
        "json": {
            "a": "1"
        },
        "multiple_object": [
            {
                "id": 63,
                "url": "http://localhost:8001/api/virtualization/clusters/63/",
                "display": "cluster1",
                "name": "cluster1"
            },
            {
                "id": 64,
                "url": "http://localhost:8001/api/virtualization/clusters/64/",
                "display": "cluster2",
                "name": "cluster2"
            }
        ],
        "multiple_selection": [
            "choice1",
            "choice2"
        ],
        "object": {
            "id": 95,
            "url": "http://localhost:8001/api/dcim/sites/95/",
            "display": "site1",
            "name": "site1",
            "slug": "site1"
        },
        "selection": "choice1",
        "text": "this is a text string",
        "text_long": "this is a really long text string\r\nwith \r\nenters",
        "url": "http://test.com"
    }
}

The implementation in this PR should handle this full structure. But the user would have to understand the structure to use it appropriately. For example

resource "netbox_device" "test" {
  name = "test"
  custom_fields = jsonencode({
    boolean = true
    text = "this is text"
    url = "http://test.com"
    multiple_selection = ["choice1", "choice2"]
      object = {
        id = 95
      }
    multiple_object = [{
        id = 63 
      },
      {
        id = 64
      }]
})
}

@fbreckle fbreckle marked this pull request as draft August 3, 2023 07:39
@tagur87
Copy link
Contributor Author

tagur87 commented Aug 12, 2023

Hey @fbreckle any further comments or thoughts on this potential implementation?

@gillg
Copy link

gillg commented Aug 14, 2023

@tagur87 I would have an opinion about your suggested exemple.

Idealy if the custom_fields json could be smart and handle id OR slug for the objects that could be nice.
Both are exclusive, and if an object type doesn't support a slug only the id is possible implicitely.

Except that is soudns perfect because it covers all the cases with a high flexibility for any future changes in netbox itself.
But I think a good data validation and error handling is important to let know the user when something fails and not be in a case where you get an error like ERROR: Invalid custom_fields

@tagur87
Copy link
Contributor Author

tagur87 commented Aug 14, 2023

Hi @gillg

Not sure what ID or slug you are referring to? Custom fields do not have those that I am aware of. It is an arbitrary map of values with an unknown structure depending on the specific configuration.

Can you clarify what you mean? Sounds like you are in agreement with this option?

Not sure what added validation we can provide...the api already does a pretty good job at responding with an error message that indicates if the custom field is not valid or improperly formatted.

@gillg
Copy link

gillg commented Aug 16, 2023

Not sure what ID or slug you are referring to? Custom fields do not have those that I am aware of. It is an arbitrary map of values with an unknown structure depending on the specific configuration.

I mean it would be bery nice to have a way to do that:

resource "netbox_device" "test" {
  name = "test"
  custom_fields = jsonencode({
    boolean = true
    text = "this is text"
    url = "http://test.com"
    multiple_selection = ["choice1", "choice2"]
    object = {
        id = 95  # Let's say this object doesn't have a secondary key like "slug" or we use a datasource to get the ID
    }
    multiple_object = [{
        slug = "key1"  # Let's say this object type is a tenant with a slug which is a valid unique key
      },
      {
        slug = "key42"
      }]
})
}

You can't define slug and id in the same map, but having the choice seems nice.
If you think it should go to a next step, sounds good to me.

@tagur87
Copy link
Contributor Author

tagur87 commented Aug 16, 2023

Not sure what ID or slug you are referring to? Custom fields do not have those that I am aware of. It is an arbitrary map of values with an unknown structure depending on the specific configuration.

I mean it would be bery nice to have a way to do that:

resource "netbox_device" "test" {
  name = "test"
  custom_fields = jsonencode({
    boolean = true
    text = "this is text"
    url = "http://test.com"
    multiple_selection = ["choice1", "choice2"]
    object = {
        id = 95  # Let's say this object doesn't have a secondary key like "slug" or we use a datasource to get the ID
    }
    multiple_object = [{
        slug = "key1"  # Let's say this object type is a tenant with a slug which is a valid unique key
      },
      {
        slug = "key42"
      }]
})
}

You can't define slug and id in the same map, but having the choice seems nice. If you think it should go to a next step, sounds good to me.

I see what you’re saying. The custom field “schema” is just a json blob that gets sent to the api. You can format it in whatever way the api will allow. If the custom field supports setting via a slug, it will work. Whatever you can do with a POST or PUT in the api will work with this field.

@fbreckle
Copy link
Collaborator

Hi again,

finally revisiting this now, since I have some time the upcoming week. I hope we can get this feature across the finish line in that time.

This time, I want to take a look at it from the HCL side of things:

Can we somehow move from this (your code):

resource "netbox_device" "test" {
  name = "test"
  custom_fields = jsonencode({
    boolean = true
    text = "this is text"
    url = "http://test.com"
    multiple_selection = ["choice1", "choice2"]
    object = {
      id = 95
    }
    multiple_object = [
      {
        id = 63 
      },
      {
        id = 64
      }
    ]
  })
}

to this (jsonencode where necessary)

resource "netbox_device" "test" {
  name = "test"
  custom_fields = {
    boolean = true
    text = "this is text"
    url = "http://test.com" # might need jsonencode
    multiple_selection = jsonencode(["choice1", "choice2"])
    object = jsonencode({
      id = 95
    })
    multiple_object = jsonencode([
      {
        id = 63 
      },
      {
        id = 64
      }
    ])
  }
}

Can this work? It might be wonky for primitives where jsonencode(x) is not x.

I ponder so much on this because I really am not a fan of using a mandatory jsonencode in all custom fields unless absolutely necessary.

I played around a bit and the more I am testing, the more I think that your implementation is the most stable one. When dealing with strings as values, the only way to know if the provided value is intended to be json is by encoding ALL values with jsonencode.

Just rambling and updating on the state of this PR. I will also have to check the behavior in the tests and at plan time manually.

@fbreckle
Copy link
Collaborator

Ok, so one problem with this solution is that it breaks existing state, because we go from TypeMap to TypeString. This would have to be solved by a state migration function, else everyone gets a string expected error at plan time, requiring a manual intervention to the state file.

I'm considering biting the bullet and migrating this provider to the framework because of https://developer.hashicorp.com/terraform/plugin/framework/migrating/benefits#custom-attribute-types .
@tagur87 you mentioned the framework earlier. Did you use the framework already? Would it allow a more elegant solution to custom fields?

@pidreher
Copy link

Hi,
to be honest - I didn't read through all of the discussion in here and I don't have a clue about how the code works (-:

I just wanted to mention that this is equally an issue with netbox_ip_addresses
Telling by the files edited in here so far, it seems it doesn't cover that yet.

@fbreckle
Copy link
Collaborator

yes. tl;dr: custom fields need to be completely revamped, but other things keep getting in the way. like netbox breaking changes with 3.6.0 again, netbox making openapi3 spec only, hashicorp changing the recommended framework for tf providers and regular features

this PR is a stand-in for the problem as a whole and has one solution implemented for a single resource

Create functions for overahauling custom fields to support multiple
underlying types.

Create function for the schema with using TypeString instead of
TypeMap. This allows for complex json types using jsonencode/jsondecode.

Create handleCustomFieldUpdate function and tests for it. This function
takes in the results of a `d.GetChange` funtion `old` & `new`, which
allows us to compare old and new values, and update the `custom_fields`
field in the api. Add unit test to validate functionality.

handleCustomFieldRead and tests were added to handle the reading of
custom fields and setting the field to "" if all the custom fields
are nil. Adds tests to validate the functionality.
Update the data source `netbox_devices` to use the string based
schema for the `custom_fields` attribute.

This allows for any underlying type to be accessed by using the
`jsondecode()` terraform function.

For example, to access custom fields, the following code can be used.

```terraform
data "netbox_devices" "test" {
	filter {
		name  = "name"
		value = "device1"
	}
}

output "device_field" {
	value = jsondecode(data.netbox_devices.test[0].custom_fields).field1
}
```
Updates the `netbox_device` resource to allow for any custom field
type to be set. This is done by using a string field with the
`jsonencode()` and `jsondecode()` terraform functions.

This allows for any complex custom fields or data types that are needed,
since the underlying types don't matter and are unmarshalled using
map[string]interface{}.

To set custom fields using this new method, use the following syntax.

```terraform
resource "netbox_device" "test" {
  name = "device1"
  tenant_id = netbox_tenant.test.id
  role_id = netbox_device_role.test.id
  device_type_id = netbox_device_type.test.id
  site_id = netbox_site.test.id
	custom_fields = jsonencode({
		"text_field" = "81"
		"bool_field" = true
	})
}
```

NOTE: This may be a breaking change for some users.
@tagur87
Copy link
Contributor Author

tagur87 commented Nov 17, 2023

Ok, so one problem with this solution is that it breaks existing state, because we go from TypeMap to TypeString. This would have to be solved by a state migration function, else everyone gets a string expected error at plan time, requiring a manual intervention to the state file.

I'm considering biting the bullet and migrating this provider to the framework because of https://developer.hashicorp.com/terraform/plugin/framework/migrating/benefits#custom-attribute-types . @tagur87 you mentioned the framework earlier. Did you use the framework already? Would it allow a more elegant solution to custom fields?

@fbreckle - sorry for the long delay on this, been wrapped up in other projects. I understand the concern on the state issues. A state migration would be the best way forward probably.

I have worked on some framework projects, but not a ton. I have a rough understanding of it. Are you thinking we should still wait till a move toward framework is started/worked on? I know there are lots of other things on your plate, let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants